tutorial flow: onboarding tour revamp + ade-cli shell-profile install#197
Merged
tutorial flow: onboarding tour revamp + ade-cli shell-profile install#197
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Owner
Author
|
@copilot review but do not make fixes |
📝 WalkthroughWalkthroughThe PR modifies CLI lane graph rendering to display lane IDs, adds shell profile update functionality to the desktop CLI installer, restructures onboarding tours with updated copy and test coverage, updates PR notification routing, revises shared CLI guidance text, and enhances iOS PR deep-link handling with Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~28 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Copilot stopped work on behalf of
arul28 due to an error
April 25, 2026 08:32
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/main/services/cli/adeCliService.ts (1)
553-582:⚠️ Potential issue | 🟠 MajorProfile-write failure incorrectly fails the entire install.
ensureUserBinOnShellPathperformsreadFileSync/mkdirSync/appendFileSyncwithout any internal error handling, so a permission error, read-only home, or sandboxed FS will throw straight into the outercatchand returnok: falsewith the IO error — even though the installer script already succeeded and theadebinary is on disk. Users will see "install failed" and likely re-run, despite having a working CLI.Treat the profile update as a best-effort enhancement to install success: catch its errors and either skip the message or surface a softer warning, but keep
ok: true.🛡️ Suggested fix
const targetDir = path.dirname(installTargetPath(envSnapshot)); - const profilePath = ensureUserBinOnShellPath(targetDir, envSnapshot); + let profilePath: string | null = null; + try { + profilePath = ensureUserBinOnShellPath(targetDir, envSnapshot); + } catch (error) { + args.logger.warn("ade_cli.shell_profile_update_failed", { + targetDir, + error: error instanceof Error ? error.message : String(error), + }); + } const status = await getStatus();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/cli/adeCliService.ts` around lines 553 - 582, The installation flow currently treats errors from ensureUserBinOnShellPath as fatal; wrap the call to ensureUserBinOnShellPath(targetDir, envSnapshot) in its own try/catch so profile update failures are best-effort: if ensureUserBinOnShellPath throws, catch it, log a non-fatal warning (using args.logger.warn and include installerPath and error message), set profilePath to undefined/null, and continue to return ok: true when the installer (spawnAsync/resolved.installerPath) succeeded; keep existing error handling for spawnAsync failures and leave getStatus usage unchanged.
🧹 Nitpick comments (9)
apps/ade-cli/src/cli.ts (1)
2645-2651: Avoid duplicating the id whennameis missing.
namealready falls back toasString(lane.id) ?? "(unknown)"on line 2645, so a lane that has anidbut nonamewill render as<id> (id: <id>). Consider only appending the suffix when a realnameexists (or when it differs fromid).♻️ Suggested tweak
- const name = asString(lane.name) ?? asString(lane.id) ?? "(unknown)"; + const laneName = asString(lane.name); + const name = laneName ?? asString(lane.id) ?? "(unknown)"; const branch = asString(lane.branchRef) ?? ""; const status = asString(lane.status) ?? ""; const archived = asString(lane.archivedAt) ? " archived" : ""; const id = asString(lane.id); - const idSuffix = id ? ` (id: ${id})` : ""; + const idSuffix = id && laneName && laneName !== id ? ` (id: ${id})` : "";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ade-cli/src/cli.ts` around lines 2645 - 2651, The printed lane line duplicates the id when name falls back to the id; change the idSuffix logic so the "(id: ...)" suffix is only appended when the displayed name is a real distinct name (not the fallback id or "(unknown)"). Concretely, capture the raw lane.name (e.g., rawName = asString(lane.name)), compute name = rawName ?? asString(lane.id) ?? "(unknown)", compute id = asString(lane.id), and set idSuffix only when id && rawName && rawName !== id (or rawName !== undefined and rawName !== id and name !== "(unknown)"); then use that idSuffix in the lines.push call.apps/desktop/src/main/services/cli/adeCliService.ts (1)
433-442: Dedup heuristics are fragile and the message can lie about what happened.A few rough edges in the idempotency logic:
existing.includes(marker)andexisting.includes(line)use plain substring matching, so any user comment containing# ADE CLI(or a differentexport PATHline that happens to contain that substring) will short-circuit and reportprofilePathas if we wrote it, even though we didn't.existing.includes(targetDir)is broader still: if some unrelated tooling already references~/.local/bin(resolved absolute), this returns the profile path and the install message claims "added${targetDir}to${profilePath}" — but we never modified the file in this run. That's a small but real correctness issue for the user-facing message.- Consider a more anchored marker like
# >>> ADE CLI PATH >>>/# <<< ADE CLI PATH <<<(matches the convention used by conda/pyenv/etc.) and distinguishing "already managed by us" vs. "user happens to already have this on PATH" so theinstallForUsermessage can be accurate.Not a blocker, but worth tightening since this writes to a file users edit by hand.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/cli/adeCliService.ts` around lines 433 - 442, The dedup logic around marker/line/targetDir is too fuzzy; update adeCliService.ts to use an anchored block marker (e.g. "# >>> ADE CLI PATH >>>" and "# <<< ADE CLI PATH <<<") instead of the short marker variable and detect that exact block via a regex so we know the file is "managed by us" (use the new begin/end markers to find the whole block around line). Separately, detect whether targetDir is already present in any PATH export by parsing export PATH lines with a regex that treats PATH elements as colon-separated tokens (so you only match targetDir as a complete element, not substring), and treat that case as "user already has targetDir on PATH" (do not claim you wrote the file). Only append the managed block when the block is absent and targetDir is not already present; reference the existing variables profilePath, existing, line, marker and targetDir to locate where to implement these checks and the append behavior.apps/desktop/src/renderer/onboarding/tours/graphTour.ts (1)
10-15: Nit: copy assumes a focused lane is selected, but the target falls back to the canvas.Step 1 says "When you click a lane, this side panel shows what state it's in..." — that references the side panel, which only exists when
[data-tour="graph.focusedLane"]is present. When the tour falls back to[data-tour="graph.canvas"](no focused lane), the spotlight lands on the canvas and "this side panel" has no obvious referent. Consider rephrasing the second sentence to describe what happens when a lane is clicked rather than referring to "this" panel, so the copy reads correctly under both anchor variants.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/onboarding/tours/graphTour.ts` around lines 10 - 15, Copy references "this side panel" while the tour target can fall back to the canvas; update the body text in graphTour.ts (the object with target: '[data-tour="graph.focusedLane"], [data-tour="graph.canvas"]', title: "How everything connects", body: ...) to avoid the deictic "this" and instead describe the behavior generically (e.g., "When you click a lane, a side panel shows its state and next steps" or "Clicking a lane opens a side panel showing its state and next steps") so the message reads correctly whether the anchor is a focused lane or the canvas.apps/desktop/src/renderer/onboarding/tours/firstJourneyTour.test.ts (1)
13-16: Constants duplicated across the tour and its test.
FALLBACK_MS,FALLBACK_LABEL, andFALLBACK_NOTICE_DEFAULTare hardcoded copies of the values inlined infirstJourneyTour.ts(OPTIONAL_ACTION_FALLBACK_MS,"Skip", and the long fallback notice string). If those defaults ever change in the source, this test will silently drift out of sync. Consider exporting the defaults fromfirstJourneyTour.ts(or a shared constants module) and importing them here so the test asserts against the actual runtime value.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/onboarding/tours/firstJourneyTour.test.ts` around lines 13 - 16, The test duplicates fallback constants (FALLBACK_MS, FALLBACK_LABEL, FALLBACK_NOTICE_DEFAULT) that are defined in the source; instead export the canonical values from firstJourneyTour.ts (e.g. OPTIONAL_ACTION_FALLBACK_MS and the fallback notice/label constants or a shared constants object) and update the test to import and assert against those exported symbols; remove the hardcoded duplicates in apps/desktop/src/renderer/onboarding/tours/firstJourneyTour.test.ts and reference the exported names so tests will follow runtime defaults.apps/desktop/src/renderer/onboarding/tours/prsTour.test.ts (2)
84-86:anycasts can useTourStepaction types.
(actions as any[]).find(...)andipcAction.call()would benefit from typing against the registry'sStepAction/TourSteptypes. Not blocking — tests work — but it would catch shape regressions inbeforeEnter's return value.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/onboarding/tours/prsTour.test.ts` around lines 84 - 86, The test currently casts actions to any[] and calls ipcAction.call() loosely; instead type the actions array using the registry types (e.g., StepAction[] or TourStep[]) and narrow the found entry with a type guard for entries where entry.type === "ipc" so ipcAction is inferred with the correct shape; update the local variables (actions and ipcAction) to use those types and call the typed ipcAction.call() (or assert its existence with expect) to catch shape regressions from beforeEnter returns.
5-5: Constants duplicated between test and source.
FALLBACK_MS = 12_000mirrors the inline constant inprsTour.ts. If the source value changes, this test will silently pass against stale data. Consider exportingFALLBACK_MSfromprsTour.tsand importing it here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/onboarding/tours/prsTour.test.ts` at line 5, The test defines a duplicated constant FALLBACK_MS (12_000) that mirrors the inline value in prsTour.ts; export FALLBACK_MS from prsTour.ts and update prsTour.test.ts to import that exported symbol instead of redefining it so the test always uses the canonical value from prsTour (remove the local FALLBACK_MS in prsTour.test.ts and replace usages with the imported FALLBACK_MS).apps/desktop/src/renderer/onboarding/tours/prsTour.ts (2)
45-50: Consider extracting the tab-switch helper.The
beforeEnterfactory pattern is duplicated verbatim between lines 45–50 and 61–66 (and again infirstJourneyTour.tsforact7Conflict/act7Checks/act10Linear). A small helper would consolidate the duplication and document the contract:♻️ Suggested helper
const dispatchPrDetailTab = (tab: "convergence" | "checks" | "overview" | "files" | "activity") => async () => [{ type: "ipc" as const, call: async () => { window.dispatchEvent(new CustomEvent("ade:tour-pr-detail-tab", { detail: tab })); }, }]; // usage beforeEnter: dispatchPrDetailTab("convergence"),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/onboarding/tours/prsTour.ts` around lines 45 - 50, The duplicated beforeEnter factory used to dispatch the "ade:tour-pr-detail-tab" CustomEvent (seen in prsTour.ts and firstJourneyTour.ts) should be extracted into a small helper like dispatchPrDetailTab(tab) that returns the async factory; replace the literal beforeEnter: async () => [...] blocks (used for tabs "convergence", "checks", "overview", "files", "activity") with beforeEnter: dispatchPrDetailTab("convergence") (etc.), ensuring the helper returns the same shape (an async function producing an array with { type: "ipc", call: async () => window.dispatchEvent(new CustomEvent("ade:tour-pr-detail-tab", { detail: tab })) }) so existing consumers of beforeEnter remain unchanged.
8-8: Magic number for fallback timeout.
FALLBACK_MS = 12_000differs from theOPTIONAL_ACTION_FALLBACK_MS = 30_000used infirstJourneyTour.ts. The shorter value is reasonable here since these steps depend on user PR selection (a quick action), while the first-journey tour waits on lane creation. Just worth a brief inline comment explaining why this tour uses a tighter timeout, so future maintainers don't accidentally unify the two.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/onboarding/tours/prsTour.ts` at line 8, FALLBACK_MS (12_000) is a magic number relative to OPTIONAL_ACTION_FALLBACK_MS (30_000) in firstJourneyTour.ts; add a brief inline comment next to the FALLBACK_MS constant explaining that this tour uses a shorter timeout because steps depend on a quick user PR selection (vs lane creation in first journey), so maintainers won't accidentally unify the two values; mention both symbols (FALLBACK_MS and OPTIONAL_ACTION_FALLBACK_MS) in the comment for context.apps/desktop/src/renderer/onboarding/tours/firstJourneyTour.ts (1)
874-892: Branches return IDs that depend on later-defined sections.
act7CloseWithBranchreturns"act6.intro"(the History intro) andact5IntroWithBranchdoes the same. Since History (act6.intro) appears after PRs in the step array (line 961), this is correct — but it's worth a one-line comment noting that"act6.intro"is the History section, not act ordinality, since the act-number/ID/list-position relationships are intentionally non-monotonic here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/onboarding/tours/firstJourneyTour.ts` around lines 874 - 892, The branch functions in act7CloseWithBranch and act5IntroWithBranch return the ID "act6.intro", which refers to the History section that is defined later in the steps array (IDs are intentionally non-monotonic); add a one-line clarifying comment above each branches function (or just above these two step definitions) stating that "act6.intro" maps to the History section and that act-number IDs do not imply list order to avoid confusion for future readers when scanning the steps array.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/renderer/onboarding/tours/firstJourneyTour.test.ts`:
- Around line 53-65: The test "derives waitForSelector from target when the
source step did not set one" currently contains a tautological assertion
comparing step.waitForSelector to itself; update the assertion to verify the
intended behavior by comparing step.waitForSelector to step.target (or remove
the redundant check) for each step in sectionSteps that has a target, and keep
the existing non-empty string assertion; look for the loop over sectionSteps and
the assertions referencing step.waitForSelector and step.target to make the
change.
In `@apps/desktop/src/renderer/onboarding/tours/firstJourneyTour.ts`:
- Around line 946-962: The section comments are mislabeled: update the block
comment that currently reads "Act 6 — PRs" to "Act 7 — PRs" above the sequence
that includes act7Intro, buildPrCreateModalWalkthrough(), act7DetailDrawer,
act7Conflict, act7Checks, act7Stacking, act7CloseWithBranch, and update the
following comment that currently reads "Act 7 — History" to "Act 6 — History"
above the lines referencing act6Intro and tutorialSection("act6.history",
historyTour.steps) so the human-readable comments match the act IDs used by the
code and tests.
- Around line 562-580: The tour steps act7Conflict and act7Checks currently
require "prCreated" and will each wait the full fallback delay when the user
takes the no-PR/noRemote path; update these steps to short-circuit when a PR
will not be created by either (a) making their PR dependency conditional—remove
"prCreated" from the static requires and add a quick runtime guard in their
beforeEnter (or call) that checks the same no-PR flag used by the noRemote path
(the modal cancel/noRemote state) and returns a skip/no-op if no PR is expected,
or (b) detect the no-PR path earlier and mark both steps as optional/omitted
(mirroring how act7CloseWithBranch is skipped). Ensure you reference
act7Conflict, act7Checks, prCreated and the noRemote branch/flag so the runtime
check matches existing logic and avoids the extra 30s fallback waits.
In `@apps/desktop/src/renderer/onboarding/tours/lanesTour.ts`:
- Around line 34-40: The step in lanesTour.ts uses a fallback selector
'[data-tour="lanes.newLane"]' which duplicates the previous step's anchor (the
"Make or adopt a lane" step) causing two consecutive spotlights; update the step
definition (the object with target/title/body/docUrl/placement) so it either (A)
gates this "Switch between lanes" step behind a runtime check for lane existence
(skip the step when no lanes exist) by consulting the same state used to render
lanes before adding this step to the tour, or (B) change the fallback selector
to a distinct element such as '[data-tour="lanes.statusChips"]' or the lanes
page container to avoid overlapping targets; modify the code that builds/returns
the steps array in lanesTour.ts (the step object with target:
'[data-tour="lanes.laneTab"], [data-tour="lanes.newLane"]') accordingly.
In `@apps/desktop/src/renderer/onboarding/tours/prsTour.test.ts`:
- Line 72: The test description for the it(...) case is stale ("Path to Merge")
and should match the current step title used in prsTour.ts and the
findStep("What's blocking me?") call; update the it(...) string to reference
"What's blocking me?" (or otherwise mirror the exact step title in prsTour.ts)
so the test description and the findStep call are consistent.
In `@apps/desktop/src/renderer/onboarding/tours/runTour.ts`:
- Around line 14-34: The tour step for target '[data-tour="run.laneSelector"]'
uses single-asterisk emphasis in its body ("*one*"), but the renderer only
supports bold via double-asterisks; update the body text in the runTour steps
(the object with target 'run.laneSelector') to replace the single-asterisk
emphasis with bold (use **one**) or remove the asterisks so it renders
correctly.
In `@apps/ios/ADE/App/DeepLinkRouter.swift`:
- Around line 72-81: The resolvePrId(from:) function currently returns the
numeric string when the identifier is numeric but the workspace snapshot is
missing or has no matching PR; change it so that when trimmed identifier parses
as an Int and either ADESharedContainer.readWorkspaceSnapshot() is nil or no
matching snapshot.prs.first(where: { $0.number == number }) is found,
resolvePrId returns nil instead of the trimmed numeric string; keep the existing
behavior for non-numeric identifiers (return trimmed) and for successful numeric
lookups (return match.id), so PrNavigationRequest.prId is only set to canonical
IDs and callers like PrsRootScreen.handleRequestedPrNavigation will receive the
correct identifier.
---
Outside diff comments:
In `@apps/desktop/src/main/services/cli/adeCliService.ts`:
- Around line 553-582: The installation flow currently treats errors from
ensureUserBinOnShellPath as fatal; wrap the call to
ensureUserBinOnShellPath(targetDir, envSnapshot) in its own try/catch so profile
update failures are best-effort: if ensureUserBinOnShellPath throws, catch it,
log a non-fatal warning (using args.logger.warn and include installerPath and
error message), set profilePath to undefined/null, and continue to return ok:
true when the installer (spawnAsync/resolved.installerPath) succeeded; keep
existing error handling for spawnAsync failures and leave getStatus usage
unchanged.
---
Nitpick comments:
In `@apps/ade-cli/src/cli.ts`:
- Around line 2645-2651: The printed lane line duplicates the id when name falls
back to the id; change the idSuffix logic so the "(id: ...)" suffix is only
appended when the displayed name is a real distinct name (not the fallback id or
"(unknown)"). Concretely, capture the raw lane.name (e.g., rawName =
asString(lane.name)), compute name = rawName ?? asString(lane.id) ??
"(unknown)", compute id = asString(lane.id), and set idSuffix only when id &&
rawName && rawName !== id (or rawName !== undefined and rawName !== id and name
!== "(unknown)"); then use that idSuffix in the lines.push call.
In `@apps/desktop/src/main/services/cli/adeCliService.ts`:
- Around line 433-442: The dedup logic around marker/line/targetDir is too
fuzzy; update adeCliService.ts to use an anchored block marker (e.g. "# >>> ADE
CLI PATH >>>" and "# <<< ADE CLI PATH <<<") instead of the short marker variable
and detect that exact block via a regex so we know the file is "managed by us"
(use the new begin/end markers to find the whole block around line). Separately,
detect whether targetDir is already present in any PATH export by parsing export
PATH lines with a regex that treats PATH elements as colon-separated tokens (so
you only match targetDir as a complete element, not substring), and treat that
case as "user already has targetDir on PATH" (do not claim you wrote the file).
Only append the managed block when the block is absent and targetDir is not
already present; reference the existing variables profilePath, existing, line,
marker and targetDir to locate where to implement these checks and the append
behavior.
In `@apps/desktop/src/renderer/onboarding/tours/firstJourneyTour.test.ts`:
- Around line 13-16: The test duplicates fallback constants (FALLBACK_MS,
FALLBACK_LABEL, FALLBACK_NOTICE_DEFAULT) that are defined in the source; instead
export the canonical values from firstJourneyTour.ts (e.g.
OPTIONAL_ACTION_FALLBACK_MS and the fallback notice/label constants or a shared
constants object) and update the test to import and assert against those
exported symbols; remove the hardcoded duplicates in
apps/desktop/src/renderer/onboarding/tours/firstJourneyTour.test.ts and
reference the exported names so tests will follow runtime defaults.
In `@apps/desktop/src/renderer/onboarding/tours/firstJourneyTour.ts`:
- Around line 874-892: The branch functions in act7CloseWithBranch and
act5IntroWithBranch return the ID "act6.intro", which refers to the History
section that is defined later in the steps array (IDs are intentionally
non-monotonic); add a one-line clarifying comment above each branches function
(or just above these two step definitions) stating that "act6.intro" maps to the
History section and that act-number IDs do not imply list order to avoid
confusion for future readers when scanning the steps array.
In `@apps/desktop/src/renderer/onboarding/tours/graphTour.ts`:
- Around line 10-15: Copy references "this side panel" while the tour target can
fall back to the canvas; update the body text in graphTour.ts (the object with
target: '[data-tour="graph.focusedLane"], [data-tour="graph.canvas"]', title:
"How everything connects", body: ...) to avoid the deictic "this" and instead
describe the behavior generically (e.g., "When you click a lane, a side panel
shows its state and next steps" or "Clicking a lane opens a side panel showing
its state and next steps") so the message reads correctly whether the anchor is
a focused lane or the canvas.
In `@apps/desktop/src/renderer/onboarding/tours/prsTour.test.ts`:
- Around line 84-86: The test currently casts actions to any[] and calls
ipcAction.call() loosely; instead type the actions array using the registry
types (e.g., StepAction[] or TourStep[]) and narrow the found entry with a type
guard for entries where entry.type === "ipc" so ipcAction is inferred with the
correct shape; update the local variables (actions and ipcAction) to use those
types and call the typed ipcAction.call() (or assert its existence with expect)
to catch shape regressions from beforeEnter returns.
- Line 5: The test defines a duplicated constant FALLBACK_MS (12_000) that
mirrors the inline value in prsTour.ts; export FALLBACK_MS from prsTour.ts and
update prsTour.test.ts to import that exported symbol instead of redefining it
so the test always uses the canonical value from prsTour (remove the local
FALLBACK_MS in prsTour.test.ts and replace usages with the imported
FALLBACK_MS).
In `@apps/desktop/src/renderer/onboarding/tours/prsTour.ts`:
- Around line 45-50: The duplicated beforeEnter factory used to dispatch the
"ade:tour-pr-detail-tab" CustomEvent (seen in prsTour.ts and
firstJourneyTour.ts) should be extracted into a small helper like
dispatchPrDetailTab(tab) that returns the async factory; replace the literal
beforeEnter: async () => [...] blocks (used for tabs "convergence", "checks",
"overview", "files", "activity") with beforeEnter:
dispatchPrDetailTab("convergence") (etc.), ensuring the helper returns the same
shape (an async function producing an array with { type: "ipc", call: async ()
=> window.dispatchEvent(new CustomEvent("ade:tour-pr-detail-tab", { detail: tab
})) }) so existing consumers of beforeEnter remain unchanged.
- Line 8: FALLBACK_MS (12_000) is a magic number relative to
OPTIONAL_ACTION_FALLBACK_MS (30_000) in firstJourneyTour.ts; add a brief inline
comment next to the FALLBACK_MS constant explaining that this tour uses a
shorter timeout because steps depend on a quick user PR selection (vs lane
creation in first journey), so maintainers won't accidentally unify the two
values; mention both symbols (FALLBACK_MS and OPTIONAL_ACTION_FALLBACK_MS) in
the comment for context.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b87a807b-0162-4b51-83b1-f9d1494d42ce
⛔ Files ignored due to path filters (6)
docs/ARCHITECTURE.mdis excluded by!docs/**docs/features/agents/README.mdis excluded by!docs/**docs/features/agents/tool-registration.mdis excluded by!docs/**docs/features/onboarding-and-settings/README.mdis excluded by!docs/**docs/features/pull-requests/README.mdis excluded by!docs/**docs/features/sync-and-multi-device/ios-companion.mdis excluded by!docs/**
📒 Files selected for processing (26)
apps/ade-cli/src/cli.test.tsapps/ade-cli/src/cli.tsapps/desktop/src/main/services/cli/adeCliService.test.tsapps/desktop/src/main/services/cli/adeCliService.tsapps/desktop/src/renderer/components/app/AppShell.tsxapps/desktop/src/renderer/components/settings/GeneralSection.tsxapps/desktop/src/renderer/onboarding/stepBuilders/createLaneDialog.tsapps/desktop/src/renderer/onboarding/stepBuilders/gitActionsPane.tsapps/desktop/src/renderer/onboarding/stepBuilders/manageLaneDialog.tsapps/desktop/src/renderer/onboarding/stepBuilders/prCreateModal.tsapps/desktop/src/renderer/onboarding/tours/automationsTour.tsapps/desktop/src/renderer/onboarding/tours/ctoTour.tsapps/desktop/src/renderer/onboarding/tours/filesTour.tsapps/desktop/src/renderer/onboarding/tours/firstJourneyTour.test.tsapps/desktop/src/renderer/onboarding/tours/firstJourneyTour.tsapps/desktop/src/renderer/onboarding/tours/graphTour.tsapps/desktop/src/renderer/onboarding/tours/historyTour.tsapps/desktop/src/renderer/onboarding/tours/laneWorkPaneTour.tsapps/desktop/src/renderer/onboarding/tours/lanesTour.tsapps/desktop/src/renderer/onboarding/tours/prsTour.test.tsapps/desktop/src/renderer/onboarding/tours/prsTour.tsapps/desktop/src/renderer/onboarding/tours/runTour.tsapps/desktop/src/renderer/onboarding/tours/settingsTour.tsapps/desktop/src/renderer/onboarding/tours/workTour.tsapps/desktop/src/shared/adeCliGuidance.tsapps/ios/ADE/App/DeepLinkRouter.swift
Comment on lines
+53
to
+65
| it("derives waitForSelector from target when the source step did not set one", () => { | ||
| for (const step of sectionSteps) { | ||
| if (step.target) { | ||
| expect(step.waitForSelector, `step ${step.id} should have waitForSelector`).toBe( | ||
| step.waitForSelector, | ||
| ); | ||
| // When target is set and source didn't override, waitForSelector === target. | ||
| // (We can't tell here whether the source set it explicitly, but at minimum it | ||
| // must be a non-empty string for any step with a target.) | ||
| expect(typeof step.waitForSelector === "string" && step.waitForSelector.length > 0).toBe(true); | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
Tautological assertion — step.waitForSelector compared to itself.
Lines 56–58 assert expect(step.waitForSelector).toBe(step.waitForSelector), which is always true and tests nothing. Per the comment on line 59, the intent is to verify that waitForSelector === target when the source step didn't override it. The follow-up assertion on line 62 covers the "non-empty string" check, so the redundant block can either be removed or rewritten to compare against step.target.
🔧 Proposed fix
it("derives waitForSelector from target when the source step did not set one", () => {
for (const step of sectionSteps) {
if (step.target) {
- expect(step.waitForSelector, `step ${step.id} should have waitForSelector`).toBe(
- step.waitForSelector,
- );
- // When target is set and source didn't override, waitForSelector === target.
- // (We can't tell here whether the source set it explicitly, but at minimum it
- // must be a non-empty string for any step with a target.)
- expect(typeof step.waitForSelector === "string" && step.waitForSelector.length > 0).toBe(true);
+ // When target is set and the source didn't override, waitForSelector === target.
+ // At minimum it must be a non-empty string for any step with a target.
+ expect(typeof step.waitForSelector).toBe("string");
+ expect((step.waitForSelector ?? "").length).toBeGreaterThan(0);
}
}
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it("derives waitForSelector from target when the source step did not set one", () => { | |
| for (const step of sectionSteps) { | |
| if (step.target) { | |
| expect(step.waitForSelector, `step ${step.id} should have waitForSelector`).toBe( | |
| step.waitForSelector, | |
| ); | |
| // When target is set and source didn't override, waitForSelector === target. | |
| // (We can't tell here whether the source set it explicitly, but at minimum it | |
| // must be a non-empty string for any step with a target.) | |
| expect(typeof step.waitForSelector === "string" && step.waitForSelector.length > 0).toBe(true); | |
| } | |
| } | |
| }); | |
| it("derives waitForSelector from target when the source step did not set one", () => { | |
| for (const step of sectionSteps) { | |
| if (step.target) { | |
| // When target is set and the source didn't override, waitForSelector === target. | |
| // At minimum it must be a non-empty string for any step with a target. | |
| expect(typeof step.waitForSelector).toBe("string"); | |
| expect((step.waitForSelector ?? "").length).toBeGreaterThan(0); | |
| } | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/renderer/onboarding/tours/firstJourneyTour.test.ts` around
lines 53 - 65, The test "derives waitForSelector from target when the source
step did not set one" currently contains a tautological assertion comparing
step.waitForSelector to itself; update the assertion to verify the intended
behavior by comparing step.waitForSelector to step.target (or remove the
redundant check) for each step in sectionSteps that has a target, and keep the
existing non-empty string assertion; look for the loop over sectionSteps and the
assertions referencing step.waitForSelector and step.target to make the change.
Comment on lines
14
to
34
| { | ||
| target: '[data-tour="run.laneSelector"]', | ||
| title: "Default lane", | ||
| body: "Processes run inside a lane's worktree. Pick which lane gets new process runs here — you can override it per-command on the card.", | ||
| title: "Where it runs", | ||
| body: "Each lane has its own copy of the project, so when you start something it runs inside *one* lane's copy. Pick the default lane here — you can override it on each command if you want.", | ||
| docUrl: docs.lanesStacks, | ||
| placement: "bottom", | ||
| }, | ||
| { | ||
| target: '[data-tour="run.stackTabs"]', | ||
| title: "Stacks", | ||
| body: "A Stack is a named group of commands you always run together — like \"dev\", \"test\", or \"deploy\". Click a tab to filter to its commands, then hit Run stack.", | ||
| title: "Group commands together", | ||
| body: "A **Stack** is a name you give to a group of commands you usually run together — like \"dev\" (your dev server + watcher), \"test\" (lint + tests), or \"deploy\". Click a tab to filter, then **Run stack** to start them all at once.", | ||
| docUrl: docs.lanesStacks, | ||
| placement: "bottom", | ||
| }, | ||
| { | ||
| target: '[data-tour="run.addCommand"]', | ||
| title: "Add a command", | ||
| body: "Define a new process — give it a name, a shell command, environment variables, a restart policy, and a readiness check. It shows up as a card immediately.", | ||
| docUrl: docs.lanesStacks, | ||
| placement: "bottom", | ||
| }, | ||
| { | ||
| target: '[data-tour="run.commandCards"]', | ||
| title: "Command cards", | ||
| body: "Each card is one process definition. The Play button starts a fresh run; the status badge and elapsed timer reflect the latest run. Click the card to edit.", | ||
| docUrl: docs.lanesStacks, | ||
| placement: "top", | ||
| }, | ||
| { | ||
| target: '[data-tour="run.runtimeBar"]', | ||
| title: "Runtime bar", | ||
| body: "Live health checks, preview URLs, and port leases for the active lane appear here so you can open your app in one click.", | ||
| body: "Save any shell command as a clickable button: a dev server, a test runner, a build script — anything. Give it a name and the command itself, and it shows up as a card you can launch any time.", | ||
| docUrl: docs.lanesStacks, | ||
| placement: "bottom", | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how `body` is rendered in the tour step component, and whether other tours already use markdown emphasis.
fd -e ts -e tsx . apps/desktop/src/renderer/onboarding | xargs rg -nP -C2 '\bbody\b'
rg -nP --type=ts -C2 '\*\*[A-Za-z]' apps/desktop/src/renderer/onboarding/toursRepository: arul28/ADE
Length of output: 50367
🏁 Script executed:
# Find the tour step renderer component
fd -e tsx -e ts . apps/desktop/src/renderer/onboarding | xargs grep -l "StepRenderer\|TourStep\|body" | head -20Repository: arul28/ADE
Length of output: 1313
🏁 Script executed:
# Look for the component that renders individual tour steps
rg -l "function.*Tour.*Step\|export.*Step.*component" apps/desktop/src/renderer/onboarding --type=tsx --type=tsRepository: arul28/ADE
Length of output: 82
🏁 Script executed:
# Check if markdown libraries are present in package.json
cat apps/desktop/package.json | grep -i "markdown\|marked\|remark"Repository: arul28/ADE
Length of output: 115
🏁 Script executed:
# Find the component that actually renders the tour step body
rg -B5 -A10 "body" apps/desktop/src/renderer/onboarding/TourController.ts | head -50Repository: arul28/ADE
Length of output: 36
🏁 Script executed:
# Search for where react-markdown is imported and used in onboarding
rg "react-markdown\|ReactMarkdown\|Markdown" apps/desktop/src/renderer/onboarding --type ts -B2 -A2Repository: arul28/ADE
Length of output: 36
🏁 Script executed:
# Read TourController.ts to see how body is rendered
wc -l apps/desktop/src/renderer/onboarding/TourController.tsRepository: arul28/ADE
Length of output: 113
🏁 Script executed:
# Look for tour step UI component files
fd "Step\|Tooltip\|Tour" -e ts apps/desktop/src/renderer/onboarding --type fRepository: arul28/ADE
Length of output: 36
🏁 Script executed:
# Read TourController.ts to see how body is rendered
cat -n apps/desktop/src/renderer/onboarding/TourController.tsRepository: arul28/ADE
Length of output: 8871
🏁 Script executed:
# Find React components in onboarding that render tour UI
fd . apps/desktop/src/renderer/onboarding -type f \( -name "*.jsx" -o -name "*.tsx" \)Repository: arul28/ADE
Length of output: 225
🏁 Script executed:
# Search for components that render the tour step body or tooltip
rg "title.*body\|<.*body\|\.body" apps/desktop/src/renderer/onboarding -A3 | head -80Repository: arul28/ADE
Length of output: 36
🏁 Script executed:
# Find all TypeScript/TSX files in onboarding
find apps/desktop/src/renderer/onboarding -type f \( -name "*.jsx" -o -name "*.tsx" \)Repository: arul28/ADE
Length of output: 36
🏁 Script executed:
# Search for tour renderer or component files more broadly
rg "TourStep\|tour.*render\|body.*render" apps/desktop/src/renderer --type ts -l | head -20Repository: arul28/ADE
Length of output: 36
🏁 Script executed:
# List all files in the onboarding directory
ls -la apps/desktop/src/renderer/onboarding/Repository: arul28/ADE
Length of output: 663
🏁 Script executed:
# Look for components that might render tours
find apps/desktop/src/renderer -name "*Tour*" -o -name "*tour*" | grep -v node_modulesRepository: arul28/ADE
Length of output: 2142
🏁 Script executed:
# Read the TourStep component which renders the body
cat -n apps/desktop/src/renderer/components/onboarding/tour/TourStep.tsxRepository: arul28/ADE
Length of output: 13978
Fix italic emphasis in tour body — only bold (**text**) is currently supported.
The tour step renderer parses only double-asterisk emphasis (**text**) to <strong> elements. Single asterisks (*text*) are rendered literally. The *one* in the body at line 17 will appear to users as *one* instead of italicized text. Either replace with bold (**one**) or remove the markdown formatting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/renderer/onboarding/tours/runTour.ts` around lines 14 - 34,
The tour step for target '[data-tour="run.laneSelector"]' uses single-asterisk
emphasis in its body ("*one*"), but the renderer only supports bold via
double-asterisks; update the body text in the runTour steps (the object with
target 'run.laneSelector') to replace the single-asterisk emphasis with bold
(use **one**) or remove the asterisks so it renders correctly.
Detail-drawer steps now carry fallback skip paths and dispatch ade:tour-pr-detail-tab to switch tabs in-flight. These invariants are load-bearing for the "user can never get stuck on a tutorial step" contract and easy to break in future edits, so pin them. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- adeCliService: distinguish "added vs already present" install message via
ShellPathResult { profilePath, modified }; handle fish shell explicitly
with fish_add_path in ~/.config/fish/config.fish (Greptile P2 #1, #3)
- DeepLinkRouter.swift: return nil from resolvePrId when numeric lookup
misses the workspace snapshot, so PrsRootScreen can degrade gracefully
instead of silently dropping the deep-link nav (Greptile P2 #2 / CR #7)
- firstJourneyTour.test.ts: drop tautological waitForSelector self-compare;
assert string + non-empty length directly (CR #1)
- firstJourneyTour.ts: swap "Act 6 — PRs" / "Act 7 — History" comments to
match underlying act7Intro/act6Intro IDs already referenced by tests (CR #3)
- lanesTour.ts: change "Switch between lanes" fallback selector from
lanes.newLane (overlapping prior step) to lanes.statusChips (CR #4)
- prsTour.test.ts: rename test description from stale "Path to Merge" to
current "What's blocking me?" step title (CR #5)
Tests: 29/29 scoped pass (adeCliService + firstJourney + prsTour).
Typecheck: clean. Swift parse: clean.
Addressed comment ids: 3141711542, 3141711558, 3141711581, 3141713614,
3141713631, 3141713638, 3141713642, 3141713651
arul28
force-pushed
the
ade/tutorial-flow-b56d5e19
branch
from
455b2fc to
2e5c89e
Compare
Owner
Author
|
@copilot review but do not make fixes |
Copilot stopped work on behalf of
arul28 due to an error
April 25, 2026 08:54
arul28
added a commit
that referenced
this pull request
Apr 25, 2026
1. Wait for BOTH signals before fixing. Review-comment edits routinely cause new CI failures, so applying them on a partial signal wastes the prior CI cycle. The "one push per iteration" bullet now spells out the *why* and the parallel-dispatch flow. 2. Normalize SINCE to UTC Z form before passing to jq. git show --format=%cI returns local-tz strings; GitHub returns …Z. jq's string-compare on 04:52:10-04:00 < 08:52:10Z (lexicographic) silently flags every old comment as "new". Phase 1.2 now pipes through python's astimezone(utc).strftime so the filter is correct. 3. Done-clean means merge, not stop. New Phase 3c (auto-merge) with squash → --admin → --auto fallback chain. Phase 5.2 + Exit-states table updated so the iteration cap routes through 3c, only landing in done-max when all three merge paths are genuinely blocked. 4. Don't pass --delete-branch to gh pr merge. It triggers a local checkout that fails when main is in another worktree (always true for /shipLane runs from lane worktrees). Phase 3c.4 documents server-side ref delete via gh api -X DELETE. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
id:suffix; adeCliService shell-profile install path with HOME/SHELL/dedup/marker safety.buildPrsRouteSearchfor PR detail navigation.Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Greptile Summary
This PR revamps the onboarding tour system (firstJourney + per-feature tours) with anchored steps, tutorialSection fallback wrapping, and PR detail-tab convergence, while also adding
ade-clishell-profile PATH injection (zsh/bash/fish/fallback) with dedup/marker safety and updated install messages. PR toast notifications inAppShellare rerouted from the lanes hash URL to the new/prsroute viabuildPrsRouteSearch, andDeepLinkRouter.swiftgains a stableprIdstring fast-path before the numeric lookup.Confidence Score: 5/5
Safe to merge — all previously flagged issues have been addressed and the new logic is well-covered by tests.
No P0/P1 findings. The three issues from prior review rounds (misleading dedup message, fish shell fallback, iOS numeric-string prId) are all resolved in this revision. New functionality is covered by 207 lines of service tests and 212 lines of tour tests. TypeScript and lint checks are reported green.
No files require special attention.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[installForUser] --> B[Run installer script] B --> C{result.status === 0?} C -- No --> D[throw Error] C -- Yes --> E[installTargetPath env] E --> F[ensureUserBinOnShellPath targetDir] F --> G{Windows?} G -- Yes --> H[return null] G -- No --> I{targetDir already on PATH?} I -- Yes --> H I -- No --> J[shellProfilePath env] J --> K{SHELL ends with?} K -- zsh --> L[~/.zshrc posix] K -- bash --> M[~/.bashrc posix] K -- fish --> N[~/.config/fish/config.fish fish] K -- other --> O[~/.profile posix] L & M & N & O --> P[shellPathEntry targetDir] P --> Q{existing includes marker/line/targetDir?} Q -- Yes --> R[return profilePath modified:false] Q -- No --> S[appendFileSync marker + line] S --> T[return profilePath modified:true] T & R --> U{profileResult?} H --> U U -- null --> V[message: PATH already on env OR plain install] U -- modified:true --> W[message: added targetDir to profilePath] U -- modified:false --> X[message: PATH entry already present in profilePath]Reviews (2): Last reviewed commit: "ship: iter 1 — rebase on main + address ..." | Re-trigger Greptile